-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Infra] Change host count KPI query #188950
[Infra] Change host count KPI query #188950
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this change Jenny. I had a quick look but haven't had the chance to test it yet.
{ | ||
term: { | ||
'event.module': 'system', | ||
}, | ||
}, | ||
{ | ||
term: { | ||
'metricset.module': 'system', | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use termQuery
helper functions. Also, how about creating constants for the field names?
{ | |
term: { | |
'event.module': 'system', | |
}, | |
}, | |
{ | |
term: { | |
'metricset.module': 'system', | |
}, | |
}, | |
...termQuery(EVENT_MODULE, 'system') | |
...termQuery(METRICSET_MODULE, 'system'), |
...queryBool, | ||
filter: [ | ||
...queryFilter, | ||
...rangeQuery, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use the rangeQuery
helper function here.
...rangeQuery, | |
...rangeQuery(new Date(from).getTime(), new Date(to).getTime()) |
|
||
framework.registerRoute( | ||
{ | ||
method: 'post', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we this could be a GET request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to pass the query, filters, time range, etc. so we can even expand more the body if it is a POST. I am not sure how much will those params extend in the future so I was thinking that having the consistency with the other endpoints with similar functions can help here. But I don't have a strong opinion on that. Do you think it's better to have query params instead of the request body and convert to GET?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GET operation is preferable in this case, since nothing is being created. Infra uses POST
for historical reasons, but most of them should be GET
instead. The complicated part is on how to pass the filters in the query params.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed I created an issue to investigate the option to use GET instead of POST in our APIs #189170 There are some benefits and concerns described there and it would be nice also to keep them consistent and have GET requests where possible ( I totally agree that it will be better but we still need to check if the url limitations affect us) So this change won't be part of this PR
export const EVENT_MODULE = 'event.module'; | ||
export const METRICSET_MODULE = 'metricset.module'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps these could be in the constants in common
instead. It will allow the client to also access them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, thanks ✅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGMT! Thanks for this change.
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
Closes #188757
Summary
This PR adds an endpoint to get the hosts (monitored by the system integration) count. Currently, it supports only hosts but it can be extended to other asset types (if/when needed). So the endpoint ( POST /api/infra/{assetType}/count ) supports only 'host' as
{assetType}
.Testing
It can't be tested in the UI so we can test it:
Using curl:
In case of testing with oblt replace the
elastic:changeme
with your user and password